-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add support for extended search usage telemetry #135306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for extended search usage telemetry #135306
Conversation
|
Hi @kderusso, I've created a changelog YAML for you. |
x-pack/plugin/inference/build.gradle
Outdated
| restResources { | ||
| restApi { | ||
| include '_common', 'bulk', 'indices', 'inference', 'index', 'get', 'update', 'reindex', 'search', 'field_caps', 'capabilities' | ||
| include '_common', 'bulk', 'cluster', 'indices', 'inference', 'index', 'get', 'update', 'reindex', 'search', 'field_caps', 'capabilities' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required for yaml test
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
|
Pinging @elastic/search-relevance (Team:Search - Relevance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level review for now. All my feedback at this time revolves around the data structure we use in ExtendedSearchUsageStats. I think we want to plan for the future here and allow for variable levels of extended stats.
server/src/main/java/org/elasticsearch/action/admin/cluster/stats/ExtendedSearchUsageStats.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review, I'll go through the rest tomorrow. Thank you for integrating the ExtendedSearchUsageMetric interface ❤️
...er/src/main/java/org/elasticsearch/action/admin/cluster/stats/ExtendedSearchUsageMetric.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/action/admin/cluster/stats/ExtendedSearchUsageLongCounter.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/action/admin/cluster/stats/ExtendedSearchUsageLongCounter.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/cluster/stats/ExtendedSearchUsageStats.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/cluster/stats/ExtendedSearchUsageStats.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/cluster/stats/ExtendedSearchUsageStats.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/ActionModule.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I left a few nit/cleanup comments, but nothing blocking.
We would need to make some changes in SearchUsage and SearchUsageHolder to use anything besides ExtendedSearchUsageLongCounter, but that's fine for now. The important thing is that the named writeable approach gives us a good way to manage serialization of future extended search usage metrics.
Adds an extended section to search usage statistics, so that we can report on the usage of specific features underneath each category. The first category we will support is
retrieversand specifically content supported by thetext_similarity_rerankerbut this leaves the option open to easily add not only other retrievers but also other high level categories, or other search data.Example output of
GET _cluster/stats?human&filter_path=indices.search*: